Conversation
| expect(diffCount).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| test("verifies gradient transformations are correct", () => { |
| const transform = ctx.getTransform(); | ||
| expect(transform).toBeDefined(); | ||
| } else { | ||
| // Should throw in fallback mode |
There was a problem hiding this comment.
is this every being run?
| // Check if the context supports transforms | ||
| const supportsTransforms = typeof _context.setTransform === "function"; | ||
|
|
||
| if (supportsTransforms) { |
There was a problem hiding this comment.
setTransform with values has been supported on every major browser for 10-15 years and should be supported in headless as well. Not worth the extra maintenance cost
There was a problem hiding this comment.
Agreed. I think we should remove this but I wanted to test that this actually solves the problem before spending any more time on far canvas :)
|
|
||
| - ✅ Supports `translate()`, `rotate()`, `scale()`, `transform()`, `setTransform()`, and `resetTransform()` | ||
| - ✅ Supports `getTransform()` to retrieve the current transformation matrix | ||
| - ✅ Leverages hardware-accelerated native Canvas transforms for better performance |
There was a problem hiding this comment.
mention of hardware-accelerated transforms might be a bit misleading here
| notImplementedYet("createImageData(imagedata)"); | ||
| // ImageData. This object is not scaled by canvas transforms directly. | ||
| // If the user passes an ImageData object, it implies screen pixels. | ||
| // Or does it? If it's from another far-canvas, it might represent world data. |
There was a problem hiding this comment.
feels a bit dangerous, better to throw than to have a don't know
| const transformStack = []; | ||
|
|
||
| // Current user transform (starts as identity) | ||
| let userTransform = createMatrix(); |
There was a problem hiding this comment.
| let userTransform = createMatrix(); | |
| let transform = createMatrix(); |
No need to introduce a new concept "user", transform without prefix is normally used for "current", this "level" which it is
There was a problem hiding this comment.
opengl uses the name "current". currentTransform makes sense
| }; | ||
|
|
||
| // Apply matrix to a point | ||
| const transformPoint = (matrix, x, y) => ({ |
| }; | ||
|
|
||
| // Invert a transform matrix | ||
| const invertMatrix = (m) => { |
| ## The solution | ||
|
|
||
| Far Canvas is a wrapper around the HTML5 2D canvas API that avoids precision issues at large coordinates. | ||
|
|
There was a problem hiding this comment.
Now that only {x, y, scale} supports big values and not transform it has to be super clear and kinda early that it's the case. Not sure where to put it
| function render(ctx) { | ||
| images.forEach((image, i) => { | ||
| function render(ctx, currentFocusValue, isReference = false) { | ||
| if (isReference) { |
There was a problem hiding this comment.
couldn't figure out why this is clear for reference
| image.data.onload = function () { | ||
| function render(ctx) { | ||
| images.forEach((image, i) => { | ||
| function render(ctx, currentFocusValue, isReference = false) { |
There was a problem hiding this comment.
should include at least some of the new functionality. Really hard to know if you have missed some test unless you look with your eyes. Most weird things I have found came from just looking at the example
|
These are good review comments ❤️ I will merge first and check that this works, if it does then I will get back to improving this Otherwise, far canvas doesn't seem to be needed anymore for most browsers. It's only mac with chrome that is a problem |
2 participants
No description provided.